Conversation
📝 WalkthroughWalkthroughThe IsNotBanned method now enforces consistent ban logic within the Findtime window. When an IP's retry count reaches MaxRetry, it's marked as banned with a new timestamp; otherwise, access is allowed with metadata updated. This mirrors the existing ShouldAllow method's behavior. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/fail2ban/fail2ban.go (1)
117-117:⚠️ Potential issue | 🟠 MajorUpdate IsNotBanned documentation to reflect its incrementing behavior, and resolve double-counting in request flow.
The comment at line 117 states "Non-incrementing check" but the code increments
Countat lines 144, 169, and 181. This is factually incorrect.Additionally, double-counting occurs in the request flow:
IsNotBannedis called in handler.go:29, and if the request is not banned, it continues to status.go:59 whereShouldAllowis called. Both functions incrementCount, causing a single request to be counted twice for a new IP (first by IsNotBanned at line 181, then by ShouldAllow at line 97).Update the documentation to accurately describe
IsNotBannedas an incrementing check, or refactor to clarify the intended semantics and prevent the double-counting behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/fail2ban/fail2ban.go` at line 117, The comment for IsNotBanned is wrong and the request is being double-counted because both IsNotBanned and ShouldAllow increment the same Count; fix by making a single canonical increment path: update IsNotBanned's docstring to reflect whether it increments or not, and refactor so only one function increments Count (recommended: remove/inhibit increments inside IsNotBanned and perform the increment inside ShouldAllow or in the request handler), or add a flag/parameter to IsNotBanned (e.g., skipIncrement bool) to prevent it from incrementing when called by the handler; ensure Count increment logic only lives in the chosen function (IsNotBanned or ShouldAllow) and update tests/comments accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/fail2ban/fail2ban.go`:
- Around line 165-189: When the findtime window has elapsed the code currently
returns true without resetting the IP entry, so future calls never re-enter the
counting window; update the else-path (the branch after the
utime.Now().Before(...) check that prints "welcome back") to reset
u.IPs[remoteIP] using an ipchecking.IPViewed struct (set Viewed to utime.Now(),
Count to 0, Denied to false) before returning true so the IP state is cleared
the same way ShouldAllow does.
---
Outside diff comments:
In `@pkg/fail2ban/fail2ban.go`:
- Line 117: The comment for IsNotBanned is wrong and the request is being
double-counted because both IsNotBanned and ShouldAllow increment the same
Count; fix by making a single canonical increment path: update IsNotBanned's
docstring to reflect whether it increments or not, and refactor so only one
function increments Count (recommended: remove/inhibit increments inside
IsNotBanned and perform the increment inside ShouldAllow or in the request
handler), or add a flag/parameter to IsNotBanned (e.g., skipIncrement bool) to
prevent it from incrementing when called by the handler; ensure Count increment
logic only lives in the chosen function (IsNotBanned or ShouldAllow) and update
tests/comments accordingly.
| if utime.Now().Before(ip.Viewed.Add(u.rules.Findtime)) { | ||
| if ip.Count+1 >= u.rules.MaxRetry { | ||
| u.IPs[remoteIP] = ipchecking.IPViewed{ | ||
| Viewed: utime.Now(), | ||
| Count: ip.Count + 1, | ||
| Denied: true, | ||
| } | ||
|
|
||
| fmt.Printf("%q is banned for %d>=%d request", | ||
| remoteIP, ip.Count+1, u.rules.MaxRetry) | ||
|
|
||
| return false | ||
| } | ||
|
|
||
| u.IPs[remoteIP] = ipchecking.IPViewed{ | ||
| Viewed: ip.Viewed, | ||
| Count: ip.Count + 1, | ||
| Denied: false, | ||
| } | ||
| return true | ||
| } | ||
|
|
||
| fmt.Printf("welcome back %q", remoteIP) | ||
|
|
||
| return true |
There was a problem hiding this comment.
Bug: IP state is never reset when findtime expires, preventing future bans.
When the findtime window has elapsed (the if on line 165 is false), execution falls through to line 187 which returns true without resetting Viewed or Count. Because Viewed remains stuck at its old value, every subsequent call will also fail the findtime check on line 165 — the IP can never accumulate new counts toward a ban again.
ShouldAllow correctly handles this (lines 106–114) by resetting the IP entry. The same reset is needed here.
🐛 Proposed fix: reset IP state when findtime expires
return true
}
+ u.IPs[remoteIP] = ipchecking.IPViewed{
+ Viewed: utime.Now(),
+ Count: 0,
+ Denied: false,
+ }
+
fmt.Printf("welcome back %q", remoteIP)
return trueNote: Count is set to 0 (instead of 1 in ShouldAllow) to stay consistent with this function's initial-visit logic on line 132 which also uses Count: 0.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/fail2ban/fail2ban.go` around lines 165 - 189, When the findtime window
has elapsed the code currently returns true without resetting the IP entry, so
future calls never re-enter the counting window; update the else-path (the
branch after the utime.Now().Before(...) check that prints "welcome back") to
reset u.IPs[remoteIP] using an ipchecking.IPViewed struct (set Viewed to
utime.Now(), Count to 0, Denied to false) before returning true so the IP state
is cleared the same way ShouldAllow does.
1 participant
This PR fixes the runtime ratelimit logic in the Fail2ban middleware plugin.
Summary by CodeRabbit